perf(api): read only the matched metrics' tags rows in PromQL label lookups by bcanvural · Pull Request #3603 · ClickHouse/ClickStack

bcanvural · 2026-11-10T13:05:13Z

Fixes #3602. Follow-up to #3581.

What

A PromQL label lookup with match[] filters the tags table by id IN (timeSeriesSelector(...)). The tags table is ordered by metric name, so that alone reads every tags row. When every selector names its metric (name{...}, name, or {__name__="name", ...}), the lookup now also filters on metric_name IN (...): the same rows, but only those metrics' part of the table is read. Selectors that may match several metrics keep today's behaviour.

Why

On a TimeSeries table with about 140M series (ClickHouse 26.10, schema version 8), a dropdown scoped to a ~3k-series metric took 6–16 s; with the metric name filter 0.24 s, same result (numbers in #3602).

Tests

Unit: selectorMetricName for bare, braced and __name__ selectors, and for selectors that may match several metrics (regex, negative matcher, __name__ inside a label value).
Integration: the existing timeseriesEngine.int.test.ts match[] cases (45/45 pass locally) cover the unchanged results.

…ookups A label lookup with match[] filtered the tags table by `id IN (timeSeriesSelector(...))`. The tags table is ordered by metric name, so that predicate alone reads every tags row. When every selector names its metric (`name{...}`, `name` or `{__name__="name"}`), also filter on metric_name: the same rows, but only those metrics' part of the table is read.

vercel · 2026-11-10T13:05:20Z

@bcanvural is attempting to deploy a commit to the HyperDX Team on Vercel.

A member of the Team first needs to authorize it.

changeset-bot · 2026-11-10T13:05:26Z

🦋 Changeset detected

Latest commit: 71e2aa6

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 4 packages

Name	Type
@hyperdx/api	Patch
@hyperdx/common-utils	Patch
@hyperdx/app	Patch
@hyperdx/otel-collector	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

github-actions · 2026-11-10T13:06:41Z

+export function selectorMetricName(selector: string): string | undefined {
+  const bare = /^\s*([a-zA-Z_:][a-zA-Z0-9_:]*)\s*(\{|$)/.exec(selector);
+  if (bare) return bare[1];
+  const label = /^\s*\{(?:[^}]*,)?\s*__name__\s*=\s*"((?:[^"\\]|\\.)*)"/.exec(


🔵 minor — Regex can read a __name__ inside another label's value, so the lookup silently returns nothing

The prefix (?:[^}]*,)? matches any text before a comma, including text inside a quoted label value. Take {path="/a,__name__=",env="prod"}: the prefix matches path="/a,, the closing quote of path is taken as the opening quote of __name__, and the function returns ,env=. The query then adds metric_name IN (',env='), so the lookup returns no values when it should return every series with that path. The same happens to any label value containing ,__name__= that is followed by another quoted matcher. A related gap: escape sequences are returned as written, not decoded (\\ stays as two characters). Fix: walk the matchers with a small tokenizer that skips over quoted strings, and return undefined if the __name__ value contains a backslash. Add a test for the comma case. The existing {job="__name__=\"up\""} test has no comma before __name__, so it never reaches this path.

_{Advisory. Fix if it is a small defect in code this PR changes; otherwise reply in the thread. Do not widen the PR or touch files it did not already change. How to respond}

Fixed in b7c7613. Matchers before __name__ are now matched whole, including single-quoted and backtick values, and only a plain metric name is taken. The unit tests cover both quoting styles.

github-actions · 2026-11-10T13:06:42Z

PR Review

If you are a coding agent acting for the author, read this first.
Nothing below blocks merge automatically; a maintainer decides. What they will expect:

🔴 critical or 🟠 major in code this PR adds or changes: fix before asking for review.

🔵 minor in code this PR changes: your call. Fix if small, otherwise reply.

Anything about surrounding code, or asking you to widen the change (hoist a helper,
dedupe with another file, fix other call sites): never fix here, whatever the
severity. Reply with one sentence; if it is critical, say so plainly so a human sees it.

One commit per review round. After two rounds, stop and ask a maintainer to review
scope rather than addressing more automated findings. Full rule: AGENTS.md.

1 finding(s): 🔴 0 critical · 🟠 0 major · 🔵 1 minor

1 posted as inline comment(s) on the changed lines.

_{Severity is the reviewer's own estimate and is used for ordering, not filtering. No finding blocks merge automatically; a maintainer decides.}

github-actions · 2026-11-10T13:12:39Z

Deep Review

✅ No critical issues found.

This is a tightly-scoped, well-iterated performance change. The new metric_name IN (...) AND id IN (...) filter is only emitted when every selector resolves to a concrete metric name (names.length < match.length guards the fallback), and promqlSelectorMetricName returns a name only for equality-constrained forms (leading Identifier, or an escape-free __name__="..." EqlSingle matcher), falling back to undefined — and thus the unchanged id-only predicate — on regex, negation, quoted label names, escaped values, wrappers, and parse errors. The correctness contract (metric_name IN (names) AND id IN (...) ≡ id IN (...)) holds for all spec-valid selectors. All ten prior review threads (lezer-promql module request, __name__-in-value reads, quote styles, escape fallback, mixed named/unnamed regression test) are addressed in the current code. The metric-name parameters are bound via chSql, so there is no injection surface.

🟡 P2 -- recommended

packages/api/src/controllers/__tests__/timeseriesEngine.int.test.ts:207 -- every all-named integration selector reaching the new metric_name IN (...) branch uses the bare-identifier form, so the {__name__="..."} form is proven to produce a name only by unit tests and never flows through the filtered query end to end.
- Fix: Add a labelValues case whose match uses the __name__ form resolving to a real metric, e.g. match: ['{__name__="recent_metric", job="api"}'] expecting ['api'], to pin the "same rows, only faster" invariant for that selector shape.
- _testing

Reviewers (8): correctness, adversarial, performance, testing, maintainability, previous-comments, kieran-typescript, project-standards.

Testing gaps:

No integration test locks the safe-fallback contract: a selector that resolves to undefined (parse error, non-EqlSingle op, escaped value) must cause getSeriesSelectorCondition to return ids without the metric_name filter.
No test pins that the new Prometheus quoted-metric-name syntax ({"my.metric", job="x"}, {"__name__"="up"}) returns undefined, so a future grammar change that began extracting a name from those forms would go uncaught.
No test documents behavior for contradictory/duplicate metric-name selectors (up{__name__="down"}, {__name__="a", __name__="b"}), which rely on ClickHouse's timeSeriesSelector erroring or returning empty rather than last-wins-resolving to a metric the metric_name filter would exclude.

_{No finding blocks merge automatically. A maintainer will expect P0/P1 findings in code this PR changes to be fixed; P2/P3 are your call -- fix or reply. Never fix findings about surrounding code here; reply instead. Do not widen the PR. How to respond}

The metric name was found by skipping everything before `__name__`, which could land inside a single-quoted or backtick value and narrow the lookup to the wrong metric. Match whole label matchers before it, and only take a plain metric name.

bcanvural · 2026-11-10T14:16:30Z

b7c7613 fixes the __name__ read from inside another label's value (thread on line 143) and drops the as string[]. A name that needs escaping now falls back to the plain id lookup.

Not changing:

Mixed named and unnamed selectors: labelNames({ match: ['{job="web"}', 'old_metric'] }) in the integration tests covers the fallback.
Narrowing only the named selectors: the unnamed ones can match any metric, so the lookup still reads the whole tags table for them and gains nothing.
A second selector parser: it recognises only the three forms above and falls back on anything else, so new syntax costs speed, not results.

github-actions · 2026-11-10T14:18:04Z

+
+// Whole matchers before `__name__`, so a quoted value containing
+// `__name__="x"` isn't read as one. Names that need escaping fall back.
+const NAME_MATCHER =


🔵 minor — Hand-rolled regex PromQL selector parser duplicates the lezer-promql parser already used in common-utils

Move metric-name extraction into common-utils next to injectPromqlLabelMatchers (packages/common-utils/src/core/promqlLabelInjection.ts), which already parses with @prometheus-io/lezer-promql. Walk the VectorSelector to get its name, or its single __name__= matcher or quoted name. That handles comments, Prometheus 3 quoted names ({"my.metric"}) and escaped values, which the NAME_MATCHER regex skips. This follows the REQUIRED 'grep before adding a helper' rule.

_{Advisory. Fix if it is a small defect in code this PR changes; otherwise reply in the thread. Do not widen the PR or touch files it did not already change. How to respond}

Not changing here: that parser lives in common-utils, and using it means a new export from a package this PR doesn't touch. The regex accepts only the three forms and falls back to the plain id lookup on anything else. Happy to switch if you'd prefer.

Hi @bcanvural can we create a module that uses @prometheus-io/lezer-promql in common-utils to handle this? And then call it here instead of adding this regex

Done in a34014d: promqlSelectorMetricName in common-utils/src/core/promqlSelector.ts walks the lezer-promql tree, and the regex is gone. It returns a name only for a single vector selector with a metric identifier or an __name__="..." matcher, and undefined otherwise (including parse errors), which keeps the plain id lookup.

github-actions · 2026-11-10T14:18:04Z

  });
 });
+
+describe('selectorMetricName', () => {


🔵 minor — {__name__="…"} selectors that add the new metric_name IN filter are never run end-to-end

Add cases to timeseriesEngine.int.test.ts with match: ['{__name__="recent_metric", job="api"}'] and a mix of __name__-form and bare selectors. Assert the same values as the equivalent recent_metric{job="api"} case. Right now only the unit test of selectorMetricName checks the NAME_MATCHER branch, and nothing runs its SQL against ClickHouse.

_{Advisory. Fix if it is a small defect in code this PR changes; otherwise reply in the thread. Do not widen the PR or touch files it did not already change. How to respond}

Not changing: {__name__="up"} produces the same metric_name IN filter as up, which the match[] selectors integration tests run end to end. The unit tests cover the parsing.

bcanvural · 2026-11-10T14:24:17Z

On the latest review, not changing: {job="x", __name__=~"up|down"} and {job="x", __name__!="up"} both return undefined, because the name is only taken when __name__ is followed by = and a quote.

karl-power · 2026-11-10T16:44:06Z

+
+// Whole matchers before `__name__`, so a quoted value containing
+// `__name__="x"` isn't read as one. Names that need escaping fall back.
+const NAME_MATCHER =


Hi @bcanvural can we create a module that uses @prometheus-io/lezer-promql in common-utils to handle this? And then call it here instead of adding this regex

Use @prometheus-io/lezer-promql in a common-utils module instead of a regex in the API, as asked in review.

github-actions · 2026-11-10T17:06:54Z

+  const names = match
+    .map(promqlSelectorMetricName)
+    .filter((n): n is string => n != null);
+  if (names.length < match.length) return ids;


🔵 minor — No test covers the new metric_name IN (...) clause or its fallback

Add a unit test on getSeriesSelectorCondition (or on the SQL that queryLabelValues/queryLabelNames builds) for three cases: all selectors named, which should add the metric_name filter with deduped names; one unnamed selector mixed in, which should leave only id IN (...); and two different named selectors, which should give metric_name IN (a, b). The existing timeseriesEngine.int.test.ts cases check only that results don't change, so they would still pass if the filter were never emitted (for example with an inverted names.length < match.length check), and they never exercise the mixed named/unnamed fallback.

_{Advisory. Fix if it is a small defect in code this PR changes; otherwise reply in the thread. Do not widen the PR or touch files it did not already change. How to respond}

Added in 71e2aa6: labelValues for job with ['{job="web"}', 'old_metric'] expects ['batch', 'web'], and fails if a mixed list is narrowed to the named metric. I said earlier that the labelNames test covered this. It didn't: web's label names also appear on old_metric series.

Not adding a test on the generated SQL: the filter changes only speed, not results, and the parser's cases are unit-tested.

github-actions · 2026-11-10T17:59:53Z

+  const names = match
+    .map(promqlSelectorMetricName)
+    .filter((n): n is string => n != null);
+  if (names.length < match.length) return ids;


🔵 minor — No test checks that the metric_name IN (...) filter is ever added, so the speed-up could vanish with every test still passing

By design the filter returns the same rows, so the integration tests in timeseriesEngine.int.test.ts give identical results whether it is added or not. If getSeriesSelectorCondition always returned ids, or the names.length < match.length check were flipped, every test would still pass. The only new integration test covers the case where no filter is added. Also, no integration test runs a braced {__name__="x"} selector through ClickHouse. Fix: export getSeriesSelectorCondition (or move it into a small module) and add unit tests on the generated ChSql. Check that the SQL contains metric_name IN with the deduplicated names when every selector names its metric (bare, name{...} and {__name__="x"} forms), and that it is left out when any selector does not name one.

_{Advisory. Fix if it is a small defect in code this PR changes; otherwise reply in the thread. Do not widen the PR or touch files it did not already change. How to respond}

bcanvural requested a review from a team as a code owner November 10, 2026 13:05

github-actions Bot added the external label Nov 10, 2026

github-actions Bot reviewed Nov 10, 2026

View reviewed changes

karl-power self-requested a review November 10, 2026 14:20

karl-power requested changes Nov 10, 2026

View reviewed changes

refactor: read the selector's metric name with the PromQL parser

a34014d

Use @prometheus-io/lezer-promql in a common-utils module instead of a regex in the API, as asked in review.

github-actions Bot reviewed Nov 10, 2026

View reviewed changes

test(api): cover a match[] list mixing named and unnamed selectors

71e2aa6

github-actions Bot reviewed Nov 10, 2026

View reviewed changes

Conversation

bcanvural commented Nov 10, 2026

What

Why

Tests

Uh oh!

vercel Bot commented Nov 10, 2026

Uh oh!

changeset-bot Bot commented Nov 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🦋 Changeset detected

Uh oh!

github-actions Bot Nov 10, 2026

Choose a reason for hiding this comment

Uh oh!

bcanvural Nov 10, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented Nov 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Review

Uh oh!

github-actions Bot commented Nov 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Deep Review

🟡 P2 -- recommended

Uh oh!

bcanvural commented Nov 10, 2026

Uh oh!

github-actions Bot Nov 10, 2026

Choose a reason for hiding this comment

Uh oh!

bcanvural Nov 10, 2026

Choose a reason for hiding this comment

Uh oh!

karl-power Nov 10, 2026

Choose a reason for hiding this comment

Uh oh!

bcanvural Nov 10, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Nov 10, 2026

Choose a reason for hiding this comment

Uh oh!

bcanvural Nov 10, 2026

Choose a reason for hiding this comment

Uh oh!

bcanvural commented Nov 10, 2026

Uh oh!

karl-power Nov 10, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Nov 10, 2026

Choose a reason for hiding this comment

Uh oh!

bcanvural Nov 10, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Nov 10, 2026

Choose a reason for hiding this comment

Uh oh!

This branch has not been deployed

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

changeset-bot Bot commented Nov 10, 2026 •

edited

Loading

github-actions Bot commented Nov 10, 2026 •

edited

Loading

github-actions Bot commented Nov 10, 2026 •

edited

Loading